feat!: remove Stack v1 support and all v1 components#294
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRemoved legacy deployment/locking and batching types/fields; consolidated ledger install paths by eliminating v2/multi-strategy branching; deleted several embedded assets and Search reconciliation; replaced many prefix-based env-var helpers with fixed keys; reworked Benthos to use resources/templates ConfigMaps with a computed config-hash; updated deepcopy, init logic, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler
participant BenthosController
participant ConfigMapStore as ConfigMap
participant K8s as Kubernetes
Reconciler->>BenthosController: Reconcile(Benthos / BenthosStream)
BenthosController->>BenthosController: Read builtinTemplates (embed.FS)
BenthosController->>BenthosController: Merge spec.Resources + spec.Templates + builtins
BenthosController->>ConfigMapStore: CreateOrUpdate(resources ConfigMap)
BenthosController->>ConfigMapStore: CreateOrUpdate(templates ConfigMap)
BenthosController->>BenthosController: Compute SHA-256 -> base64 ConfigMapHash
BenthosController->>K8s: Update BenthosStream.Status.ConfigMapHash
BenthosController-->>Reconciler: Return
sequenceDiagram
participant LedgerReconciler
participant LedgerInstaller
participant Database
LedgerReconciler->>LedgerInstaller: installLedger(stack, ledger, database, imageConfiguration, version)
LedgerInstaller->>LedgerInstaller: installLedgerSingleInstance()
LedgerInstaller->>LedgerInstaller: createBaseLedgerContainer()
LedgerInstaller->>LedgerInstaller: setCommonContainerConfiguration()
LedgerInstaller->>LedgerInstaller: createDeployment()
alt Version upgrade needed
LedgerInstaller->>LedgerInstaller: getUpgradeContainer()
LedgerInstaller->>LedgerInstaller: migrate()
LedgerInstaller->>Database: Save module version
end
LedgerInstaller-->>LedgerReconciler: Return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (17)
📒 Files selected for processing (33)
Comment |
flemzord
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
api/formance.com/v1beta1/ledger_types.go (1)
20-20: Remove unused import.The
timepackage is imported but not used in the file, causing a compilation error.Apply this diff to fix the compilation error:
-import ( - "time" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +)internal/resources/ledgers/deployments.go (1)
70-71: Fix function signature to use image parameter.The function should receive
imageinstead ofversionto be consistent with other deployment functions and fix the compilation error.Apply this diff to fix the function signature:
func installLedgerStateless(ctx core.Context, stack *v1beta1.Stack, - ledger *v1beta1.Ledger, database *v1beta1.Database, version string) error { + ledger *v1beta1.Ledger, database *v1beta1.Database, image string) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_volumes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (6)
api/formance.com/v1beta1/ledger_types.go(1 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(0 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/assets/Caddyfile.gotpl(0 hunks)internal/resources/ledgers/deployments.go(8 hunks)internal/resources/ledgers/init.go(3 hunks)
💤 Files with no reviewable changes (3)
- internal/resources/ledgers/assets/Caddyfile.gotpl
- internal/resources/ledgers/assets.go
- api/formance.com/v1beta1/zz_generated.deepcopy.go
🧰 Additional context used
🪛 GitHub Actions: Default
api/formance.com/v1beta1/ledger_types.go
[error] 20-20: GolangCI-Lint typecheck error: "time" imported and not used.
🔇 Additional comments (5)
api/formance.com/v1beta1/ledger_types.go (1)
40-41: LGTM!The simplified description accurately reflects the ledger's purpose after removing multiple deployment strategies.
internal/resources/ledgers/init.go (1)
86-106: LGTM!The simplified migration logic correctly handles version changes while maintaining backward compatibility for versions >= v2.2.0-alpha.
internal/resources/ledgers/deployments.go (3)
30-49: LGTM!The simplified deployment logic correctly handles different ledger versions while removing v1-specific code paths.
290-306: LGTM!The consistent removal of environment variable prefixes standardizes the configuration approach across all ledger deployments.
260-285: LGTM!The simplified deployment creation correctly removes v1-specific volume logic, making the code cleaner and more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/09-Configuration reference/02-Custom Resource Definitions.md (1)
744-745: Add an explicit migration note for removed v1-only fieldsThe new description is concise, but existing users upgrading from v1 will likely look here for guidance. A short, in-place note that
deploymentStrategyandlockingfields are no longer supported would prevent silent confusion.The ledger is a stateful application that manages financial transactions and maintains an immutable audit trail. + +> **Important:** Starting with Stack v2, the legacy `deploymentStrategy` +> and `locking` fields have been removed. Manifests that still declare +> these properties will be rejected by the operator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_ledgers.formance.com.yamlis excluded by!**/*.yaml
📒 Files selected for processing (3)
api/formance.com/v1beta1/ledger_types.go(1 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(1 hunks)internal/resources/ledgers/init.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/formance.com/v1beta1/ledger_types.go
- internal/resources/ledgers/init.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
flemzord
changed the title
1b82b3c to
10bbb5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/resources/ledgers/assets.go (1)
7-8: Verify that v2.0.0 reindex assets exist and are populated.The embed directive now exclusively targets
assets/reindex/v2.0.0/*.yaml. Since this is a breaking change removing v1 support, ensure that:
- The directory
internal/resources/ledgers/assets/reindex/v2.0.0/exists and contains the required YAML files- These assets are sufficient for all reindex scenarios
Run the following script to verify the reindex assets exist:
#!/bin/bash # Description: Verify that v2.0.0 reindex assets exist # Check if the directory exists and list its contents fd -t f . internal/resources/ledgers/assets/reindex/v2.0.0/ --exec ls -lh # Count the number of YAML files echo "Number of YAML files:" fd -e yaml . internal/resources/ledgers/assets/reindex/v2.0.0/ | wc -l
🧹 Nitpick comments (2)
internal/resources/ledgers/init.go (1)
85-102: Add logging for migration failures before falling back to install
Before returninginstallLedgeron anApplicationError, emit an Info-level log of the originalmigrateerror (e.g.log.FromContext(ctx).Info("ledger migration failed, falling back to install", "error", err)) to improve observability. Confirm that falling back to installation for all
ApplicationErrorcases is the intended behavior.internal/resources/ledgers/deployments.go (1)
431-460: Ensure safe deletion of deployments and add migration status
- Deleting all ledger Deployments in the PreCreate hook will tear down every live pod until the migration job finishes—confirm this aligns with your availability SLAs or adopt a rolling-orchestrated approach to avoid full downtime.
- The controller never sets a “migration in progress” condition on the Ledger CR; consider adding a Status condition or annotation so users can observe pending migrations.
jobs.Handlecorrectly returns acore.NewPendingError()andinit.gopropagates it to requeue reconciliation until the Job completes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (9)
config/crd/bases/formance.com_ledgers.yamlis excluded by!**/*.yamlhelm/crds/Chart.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_ledgers.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v1.0.0/ledger_reindex_volumes.yamlis excluded by!**/*.yaml
📒 Files selected for processing (7)
api/formance.com/v1beta1/ledger_types.go(1 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(1 hunks)internal/resources/ledgers/assets.go(1 hunks)internal/resources/ledgers/assets/Caddyfile.gotpl(0 hunks)internal/resources/ledgers/deployments.go(9 hunks)internal/resources/ledgers/init.go(3 hunks)
💤 Files with no reviewable changes (2)
- internal/resources/ledgers/assets/Caddyfile.gotpl
- api/formance.com/v1beta1/zz_generated.deepcopy.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/resources/ledgers/init.go (3)
internal/resources/benthosstreams/create.go (1)
LoadFromFileSystem(19-90)internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)internal/core/errors.go (1)
IsApplicationError(43-45)
internal/resources/ledgers/deployments.go (10)
internal/core/context.go (1)
Context(10-16)internal/core/version.go (1)
IsLower(54-62)internal/core/env.go (2)
Env(11-16)GetDevEnvVarsWithPrefix(75-84)internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/resources/settings/opentelemetry.go (1)
GetOTELEnvVarsWithPrefix(23-39)internal/core/module.go (1)
LowerCamelCaseKind(8-14)internal/resources/databases/env.go (1)
PostgresEnvVarsWithPrefix(20-104)internal/resources/brokers/utils.go (2)
GetEnvVarsWithPrefix(20-72)GetPublisherEnvVars(74-92)internal/core/stacks.go (1)
HasDependency(69-80)internal/resources/jobs/job.go (3)
Handle(137-220)PreCreate(31-35)WithServiceAccount(43-48)
🔇 Additional comments (8)
docs/09-Configuration reference/02-Custom Resource Definitions.md (1)
744-745: LGTM! Documentation simplified to match API changes.The updated Ledger description is concise and accurately reflects the removal of deployment strategy complexity. This aligns well with the API changes that removed
DeploymentStrategyandLockingfields.api/formance.com/v1beta1/ledger_types.go (1)
38-39: LGTM! API documentation updated to reflect simplified architecture.The streamlined documentation aligns with the removal of
DeploymentStrategyandLockingfields fromLedgerSpec, making the API cleaner and easier to understand.internal/resources/ledgers/init.go (1)
85-104: Approve simplified migration and installation flow.The removal of version-based branching (isV2) and the direct invocation of
migrate()when versions differ streamlines the ledger initialization logic. The unconditional call toinstallLedgerat line 104 ensures the deployment is always in sync with the desired state.internal/resources/ledgers/deployments.go (5)
30-49: LGTM! Simplified installation flow with version-based branching.The
installLedgerfunction now cleanly branches between:
- v2.2.0+ with stateless + optional worker deployment
- Older versions using single-instance deployment
The removal of the
v2parameter simplifies the API and aligns with the unified v2 architecture.
51-68: LGTM! Single-instance deployment simplified.The function signature is cleaner without the
v2parameter, and the logic now unconditionally usescreateLedgerContainerFullwith the simplified API. The call tosetCommonAPIContainerConfigurationcorrectly passesimageConfiguration(fixing the issue flagged in past reviews).
328-350: Approve simplified environment variable configuration.The removal of the
v2parameter and the NUMARY_ prefix simplifies the environment variable setup. All services now use consistent naming with empty prefixes, which is cleaner and easier to maintain.
383-429: LGTM! Unified broker environment variable handling.The
createLedgerContainerFullfunction now consistently uses empty string prefixes for all environment variables, including:
GetEnvVarsWithPrefix(..., "")at line 403GetPublisherEnvVars(..., "")at line 409This aligns with the removal of v2-specific NUMARY_ prefixes and creates a consistent environment variable naming scheme.
259-270: Validate pre-RC6 upgrade command and RC6+ default migration
- Confirm
buckets upgrade-allis available and correct for all versions < v2.0.0-rc.6 (note the webhooks deployment comments indicate upgrade support beginning in v2.0.0-rc.5).- Confirm that for v2.0.0-rc.6 and later, the default migration container uses the
migratecommand (args := ["migrate"]ininternal/resources/databases/migrate.go).
- Remove deprecated DeploymentStrategy and Locking properties from Ledger CRD - Remove all v1-specific logic and conditional code paths - Simplify ledger deployment to always use v2 behavior - Remove v1.0.0 reindex assets - Remove unused Caddyfile template for ledger - Update documentation to remove references to deployment strategies - Clean up imports and remove unused dependencies BREAKING CHANGE: This removes support for Ledger v1 deployments. All ledgers will now use v2 behavior by default.
- Simplified the description of the ledger as a stateful application managing financial transactions and maintaining an immutable audit trail. - Removed deprecated properties related to deployment strategies and locking from the Ledger CRD and documentation.
- Changed the embedded asset path to include versioning for reindex YAML files.
BREAKING CHANGE: Remove Stack v1 support and upgrade to version 3.0.0 This is a major version bump due to the removal of Stack v1 support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
10bbb5f to
493d155
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (14)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/rbac/role.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockhelm/operator/templates/gen/rbac.authorization.k8s.io_v1_clusterrole_formance-manager-role.yamlis excluded by!**/gen/**,!**/*.yaml,!**/gen/**internal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (34)
api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(2 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(2 hunks)internal/core/env.go(1 hunks)internal/core/reconciler.go(0 hunks)internal/resources/all.go(0 hunks)internal/resources/auths/env.go(2 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(4 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/brokers/utils.go(2 hunks)internal/resources/databases/env.go(3 hunks)internal/resources/gateways/caddyfile.go(0 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/deployments.go(10 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/deployments.go(1 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/deployments.go(2 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/settings/opentelemetry.go(5 hunks)internal/resources/webhooks/deployment.go(1 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (14)
- internal/core/reconciler.go
- internal/resources/ledgers/assets.go
- internal/tests/searches_controller_test.go
- docs/09-Configuration reference/01-Settings.md
- internal/tests/ledger_controller_test.go
- internal/resources/payments/init.go
- api/formance.com/v1beta1/search_types.go
- internal/resources/gateways/caddyfile.go
- internal/resources/searches/init.go
- internal/tests/payments_controller_test.go
- internal/resources/searches/benthos/fs.go
- internal/resources/all.go
- internal/resources/benthosstreams/create.go
- internal/resources/searches/clean_legacy_consumers.go
✅ Files skipped from review due to trivial changes (1)
- internal/resources/brokerconsumers/create.go
🧰 Additional context used
🧬 Code graph analysis (14)
internal/tests/benthos_controller_test.go (4)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/benthos_types.go (1)
Benthos(51-57)api/formance.com/v1beta1/broker_types.go (1)
Broker(59-65)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)
api/formance.com/v1beta1/benthos_types.go (1)
api/formance.com/v1beta1/shared.go (2)
StackDependency(284-287)DevProperties(21-31)
internal/resources/benthos/controller.go (2)
internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/resources/orchestrations/deployments.go (1)
internal/resources/brokers/utils.go (1)
GetPublisherEnvVars(70-88)
internal/resources/auths/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/resources/brokers/utils.go (4)
internal/core/env.go (1)
Env(11-16)internal/resources/settings/helpers.go (2)
Get(20-38)IsTrue(462-464)api/formance.com/v1beta1/broker_types.go (4)
Broker(59-65)Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)api/formance.com/v1beta1/shared.go (2)
Status(224-233)URI(310-312)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/auths/env.go (1)
internal/core/env.go (1)
Env(11-16)
internal/resources/orchestrations/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/payments/deployments.go (1)
internal/resources/brokers/utils.go (1)
GetPublisherEnvVars(70-88)
internal/resources/settings/opentelemetry.go (2)
internal/core/env.go (3)
Env(11-16)EnvFromBool(18-24)ComputeEnvVar(58-64)internal/resources/settings/helpers.go (1)
IsTrue(462-464)
internal/resources/databases/env.go (3)
internal/core/env.go (3)
Env(11-16)EnvFromSecret(40-52)ComputeEnvVar(58-64)api/formance.com/v1beta1/shared.go (1)
URI(310-312)internal/resources/settings/helpers.go (1)
IsTrue(462-464)
internal/resources/ledgers/deployments.go (11)
internal/core/context.go (1)
Context(10-16)internal/resources/registries/registries.go (1)
ImageConfiguration(18-23)internal/core/env.go (2)
Env(11-16)GetDevEnvVars(68-77)internal/resources/brokers/utils.go (2)
GetPublisherEnvVars(70-88)GetBrokerEnvVars(16-68)internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/resources/settings/opentelemetry.go (1)
GetOTELEnvVars(19-34)internal/core/module.go (1)
LowerCamelCaseKind(8-14)internal/resources/databases/env.go (1)
GetPostgresEnvVars(15-106)internal/resources/auths/env.go (1)
ProtectedEnvVars(12-55)internal/resources/gateways/gateways.go (1)
EnvVarsIfEnabled(15-26)internal/resources/settings/helpers.go (1)
GetInt(164-173)
🔇 Additional comments (5)
internal/resources/webhooks/init.go (1)
70-74: LGTM! Clean simplification of deployment logic.The removal of version-based branching in favor of a single deployment path when the consumer is ready is a clean simplification that aligns with the PR objective to remove v1 support. Error handling is appropriate.
internal/resources/webhooks/deployment.go (1)
127-129: LGTM! Consolidation to single deployment path.The
createSingleDeploymentfunction is now a clean wrapper that always enables worker functionality by passingwithWorker=truetocreateAPIDeployment. This consolidation eliminates the dual deployment path and aligns with the PR objective to simplify deployment logic by removing v1-specific code paths.api/formance.com/v1beta1/zz_generated.deepcopy.go (3)
24-24: LGTM! Necessary import for json.RawMessage.The
encoding/jsonimport is required for the new deepcopy logic handlingjson.RawMessagefields in BenthosSpec.
344-375: LGTM! Correct deepcopy implementation for Resources and Templates.The generated deepcopy logic properly handles
map[string]json.RawMessagefields:
- Creates new map and deeply copies each
json.RawMessagevalue- Correctly handles nil values
- Ensures proper deep copy by creating new byte slices
1280-1289: LGTM! Simplified LedgerSpec deepcopy aligns with v1 removal.The removal of Locking field handling correctly reflects the deprecation of Ledger v1 support as described in the PR objectives.
2fd8536 to
072a9d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (15)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthosstreams.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockinternal/resources/benthos/builtin-templates/event_bus.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (30)
api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/benthosstream_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(1 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(2 hunks)internal/core/reconciler.go(1 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(7 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/benthosstreams/init.go(2 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/gateways/Caddyfile.gotpl(1 hunks)internal/resources/gateways/caddyfile.go(0 hunks)internal/resources/gateways/deployment.go(2 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/gateways/init.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/webhooks/deployment.go(1 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (12)
- docs/09-Configuration reference/01-Settings.md
- internal/resources/ledgers/assets.go
- api/formance.com/v1beta1/search_types.go
- internal/resources/payments/init.go
- internal/tests/ledger_controller_test.go
- internal/resources/searches/clean_legacy_consumers.go
- internal/resources/gateways/caddyfile.go
- internal/resources/searches/benthos/fs.go
- internal/tests/payments_controller_test.go
- internal/resources/benthosstreams/create.go
- internal/tests/searches_controller_test.go
- internal/resources/searches/init.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/formance.com/v1beta1/benthos_types.go
- internal/resources/brokerconsumers/create.go
- internal/resources/webhooks/init.go
🧰 Additional context used
🧬 Code graph analysis (10)
api/formance.com/v1beta1/benthosstream_types.go (1)
api/formance.com/v1beta1/shared.go (1)
Status(224-233)
internal/resources/gateways/init.go (2)
api/formance.com/v1beta1/shared.go (1)
Status(224-233)internal/core/errors.go (1)
NewPendingError(35-37)
internal/resources/auths/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/orchestrations/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/tests/benthos_controller_test.go (3)
api/formance.com/v1beta1/benthos_types.go (1)
Benthos(50-56)api/formance.com/v1beta1/broker_types.go (1)
Broker(59-65)api/formance.com/v1beta1/stack_types.go (1)
Stack(77-83)
internal/resources/benthos/controller.go (5)
internal/resources/settings/aws_role.go (1)
GetAWSServiceAccount(5-7)internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/resources/settings/helpers.go (1)
RequireURL(92-98)internal/core/env.go (1)
Env(11-16)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/resources/benthosstreams/init.go (1)
internal/core/utils.go (1)
HashFromConfigMaps(27-35)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/gateways/deployment.go (2)
api/formance.com/v1beta1/broker_types.go (4)
Broker(59-65)Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)internal/core/env.go (1)
Env(11-16)
🔇 Additional comments (13)
internal/resources/auths/init.go (1)
85-93: CRITICAL: Inconsistent migration condition updates across related modules.The simplified migration check in auths (line 85) matches payments, orchestrations, and ledgers but diverges from webhooks and reconciliations, which still retain
IsGreaterOrEqual(version, "v2.0.0-rc.5")gates. Verify whether:
- The webhooks and reconciliations files require intentional different handling, or
- They should also be updated to the simplified condition for consistency
If this PR intends to unify migration logic across all modules post-v1 removal, webhooks and reconciliations should be updated to match the pattern now used in auths, payments, orchestrations, and ledgers.
internal/resources/gateways/Caddyfile.gotpl (1)
14-16: No issues found. The code is correctly synchronized.The original concern about a missing default value for
{$TOPIC_NAME}is invalid. The EnableAudit template data is only set to true when bothstack.Spec.EnableAuditandbrokerare non-nil, and the audit block in the Caddyfile is only included when EnableAudit is true. Correspondingly, TOPIC_NAME is only set in the deployment when broker is non-nil. This ensures that whenever thetopic_namedirective appears in the Caddyfile, theTOPIC_NAMEenvironment variable will be properly set.internal/core/reconciler.go (2)
138-138: LGTM! Improved naming consistency.The parameter rename from
buildertobaligns with similar handler functions elsewhere in the file (lines 148, 165).
174-231: Removal ofWithReconcilerverified with no orphaned references.The exported
WithReconcilerfunction has been successfully removed from the codebase with no remaining callers or references. All alternative APIs (WithStdReconciler,WithStackDependencyReconciler,WithModuleReconciler) are available and properly exported.api/formance.com/v1beta1/zz_generated.deepcopy.go (1)
343-356: LGTM! Auto-generated deepcopy code for new BenthosSpec fields.The deepcopy implementation for the Resources and Templates map fields follows the standard pattern and correctly handles map[string]string types.
internal/resources/gateways/init.go (1)
63-67: Good defensive check for broker readiness.This early return prevents proceeding with ConfigMap creation when the broker dependency is not yet ready, avoiding potential configuration issues downstream.
api/formance.com/v1beta1/benthosstream_types.go (1)
30-31: LGTM! New ConfigMapHash field added to track configuration state.The field is appropriately marked as optional and aligns with the reconciliation logic that computes and sets this hash value.
internal/resources/benthosstreams/init.go (1)
59-59: LGTM! ConfigMapHash tracking added.Computing and storing the ConfigMap hash enables detection of configuration changes and proper reconciliation.
internal/resources/gateways/gateways.go (1)
28-41: Simplified environment variable generation by removing prefix support.The removal of prefix-based environment variable naming aligns with the PR's goal to simplify the codebase by removing v1 compatibility code. The fixed variable names (STACK_URL and STACK_PUBLIC_URL) are more predictable and maintainable.
internal/resources/gateways/deployment.go (2)
13-20: Formatting change improves readability.The multi-line function signature with trailing comma is a minor style improvement.
44-57: LGTM! TOPIC_NAME injection correctly handles broker modes.The logic appropriately determines the topic prefix based on the broker's mode (OneStreamByService vs OneStreamByStack) and constructs the TOPIC_NAME environment variable accordingly. The nil check on broker prevents issues when no broker is configured.
docs/09-Configuration reference/02-Custom Resource Definitions.md (2)
710-711: Documentation simplified to remove v1 deployment strategy references.The updated description correctly reflects the removal of deployment strategy configuration options.
1614-1615: Documentation updated for new BenthosSpec fields.The Resources and Templates fields are appropriately documented as map types, aligning with the code changes.
f2ea6c5 to
cd0ad80
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
internal/resources/benthosstreams/init.go (1)
34-40: The cleanup condition is overly broad and risks deleting legitimate resources.This condition deletes any BenthosStream with exactly one owner from
formance.com/v1beta1, which matches ALL resources in this operator, not just legacy ones.Consider one of these approaches:
- Check the owner's Kind against a whitelist of deprecated kinds
- Add a legacy marker label/annotation to identify resources for cleanup
- Check creation timestamp to only remove old resources
- if len(stream.GetOwnerReferences()) == 1 && stream.GetOwnerReferences()[0].APIVersion == "formance.com/v1beta1" { + // Check if owned by a legacy/deprecated Kind or has legacy marker + if len(stream.GetOwnerReferences()) == 1 && + stream.GetOwnerReferences()[0].APIVersion == "formance.com/v1beta1" && + isLegacyOwnerKind(stream.GetOwnerReferences()[0].Kind) { return ctx.GetClient().Delete(ctx, stream) }internal/resources/ledgers/init.go (1)
107-114: Guard inversion lets broken migrations slip throughRight now
isV2_2flips the intent: any version ≥ v2.2.0‑alpha (or even an unparsable tag) skips the earlyreturn err, so we still start the new ledger binary when migrations fail. That’s the opposite of what we promised in the earlier discussion (“only keep this fallback for v2.0/v2.1”). In practice, a v2.3 rollout that hits anIsApplicationErrorwill boot the upgraded pods on an unmigrated schema, inviting runtime breakage or data corruption. We must keep the fallback only for legacy < v2.2 builds and otherwise bubble the error.Recommend tightening the guard like this:
- isV2_2 := !semver.IsValid(version) || semver.Compare(version, "v2.2.0-alpha") > 0 - if !isV2_2 { - return err - } - - if IsApplicationError(err) { // Start the ledger even if migrations are not terminated + if !semver.IsValid(version) || semver.Compare(version, "v2.2.0-alpha") >= 0 { + return err + } + + if IsApplicationError(err) { // Start the ledger even if migrations are not terminated return installLedger(ctx, stack, ledger, database, imageConfiguration, version) }That way only v2.0/v2.1-era upgrades keep the temporary escape hatch; everything newer fails fast so we don’t run against an out-of-date schema.
🧹 Nitpick comments (3)
internal/core/reconciler.go (1)
138-138: Parameter rename improves consistency.The change from
buildertobaligns with similar handlers at lines 148 and 165. For full consistency, consider updating line 113 inWithWatchSettingsto also useb.internal/resources/gateways/deployment.go (1)
44-57: Consider adding a bounds check for container access.While
caddy.DeploymentTemplateshould always create at least one container, accessingContainers[0]without a bounds check could panic in edge cases. Consider adding a defensive check:if broker != nil { var topicPrefix string switch broker.Status.Mode { case v1beta1.ModeOneStreamByService: topicPrefix = broker.Spec.Stack + "-" case v1beta1.ModeOneStreamByStack: topicPrefix = broker.Spec.Stack + "." } + if len(caddyTpl.Spec.Template.Spec.Containers) == 0 { + return errors.New("caddy deployment template has no containers") + } + caddyTpl.Spec.Template.Spec.Containers[0].Env = append( caddyTpl.Spec.Template.Spec.Containers[0].Env, core.Env("TOPIC_NAME", topicPrefix+"gateway"), ) }internal/resources/benthos/controller.go (1)
118-142: Consider adding a nil check for broker.Status.URI.While the
broker.Status.Readycheck should ensurebroker.Status.URIis populated, adding a defensive nil check would prevent potential panics:env := []corev1.EnvVar{ Env("TOPIC_PREFIX", topicPrefix), Env("STACK", b.Spec.Stack), Env("BROKER", broker.Status.URI.Scheme), } + + if broker.Status.URI == nil { + return errors.New("broker URI is not set despite ready status") + } + if awsIAMEnabled { env = append(env, Env("AWS_IAM_ENABLED", "true")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stacks.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthosstreams.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockinternal/resources/benthos/builtin-templates/event_bus.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (33)
README.md(1 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/benthosstream_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/stack_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(1 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(2 hunks)internal/core/reconciler.go(1 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(7 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/benthosstreams/init.go(2 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/gateways/Caddyfile.gotpl(1 hunks)internal/resources/gateways/caddyfile.go(1 hunks)internal/resources/gateways/deployment.go(2 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/gateways/init.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/webhooks/deployment.go(0 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(3 hunks)internal/tests/gateway_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (13)
- api/formance.com/v1beta1/search_types.go
- api/formance.com/v1beta1/stack_types.go
- internal/resources/searches/benthos/fs.go
- internal/tests/searches_controller_test.go
- internal/tests/payments_controller_test.go
- internal/resources/searches/clean_legacy_consumers.go
- internal/resources/searches/init.go
- internal/resources/payments/init.go
- internal/resources/webhooks/deployment.go
- docs/09-Configuration reference/01-Settings.md
- internal/resources/ledgers/assets.go
- internal/tests/ledger_controller_test.go
- internal/resources/benthosstreams/create.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/resources/webhooks/init.go
- internal/resources/gateways/init.go
- internal/resources/auths/init.go
- api/formance.com/v1beta1/zz_generated.deepcopy.go
- api/formance.com/v1beta1/benthos_types.go
- internal/resources/gateways/Caddyfile.gotpl
🧰 Additional context used
🧬 Code graph analysis (8)
api/formance.com/v1beta1/benthosstream_types.go (1)
api/formance.com/v1beta1/shared.go (1)
Status(224-233)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/resources/benthos/controller.go (4)
internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/resources/settings/helpers.go (2)
Get(20-38)IsTrue(462-464)internal/core/env.go (1)
Env(11-16)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/resources/benthosstreams/init.go (1)
internal/core/utils.go (1)
HashFromConfigMaps(27-35)
internal/resources/gateways/deployment.go (3)
api/formance.com/v1beta1/broker_types.go (3)
Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)internal/resources/gateways/gateways.go (1)
GetEnvVars(28-41)internal/core/env.go (2)
GetDevEnvVars(68-77)Env(11-16)
internal/tests/benthos_controller_test.go (2)
api/formance.com/v1beta1/stack_types.go (1)
Stack(72-78)api/formance.com/v1beta1/settings_types.go (1)
Settings(208-213)
internal/resources/orchestrations/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
🪛 markdownlint-cli2 (0.18.1)
README.md
111-111: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (16)
internal/resources/brokerconsumers/create.go (1)
11-11: LGTM! Good refactor to remove redundant import alias.The explicit
v1beta1alias is unnecessary since the package directory is already namedv1beta1. Go will use it as the default package name, making this refactor both correct and aligned with Go conventions.internal/resources/orchestrations/init.go (1)
66-75: I need to understand what migration tool is being used and verify its idempotency guarantees.Now I need to identify what migration tool is actually being used in this codebase. Let me search for migration-related files and dependencies.
Based on my analysis of the migration tools and the Migrate function implementation, I can now provide a more informed rewrite. The critical findings are:
- Executing migrate is idempotent, meaning the Migrate function itself is designed to be safely called multiple times.
- Flyway adds a schema history table to track which migrations have already been applied, preventing re-execution of versioned migrations.
- Flyway compares available migrations to those already applied to the database and migrates to close any gap.
However, there remains one important consideration about the scenario in the reviewed code:
Verify the migration version tracking behavior and downgrade handling.
The current logic retries migration if
SaveModuleVersionfails after a successfulMigratecall. While Flyway scans migration directories, calculates what's new, and runs pending scripts in incremental order, a version downgrade scenario (deploying an older module version) could trigger unexpected re-migration attempts. Please confirm:
- Whether the codebase uses Flyway or another migration tool that tracks applied migrations
- Whether version downgrades should be explicitly prevented or logged
- Whether all migrations follow versioned migration patterns (V prefix) that execute once and only once
docs/09-Configuration reference/02-Custom Resource Definitions.md (2)
710-711: LGTM!The enhanced description provides better context about the Ledger module's purpose and characteristics.
1614-1615: LGTM!The new
resourcesandtemplatesfields align with the migration to ConfigMap-driven Benthos configuration.internal/tests/gateway_controller_test.go (1)
196-196: LGTM!The updated context label accurately reflects that the test verifies consumer-based audit functionality rather than a stack-level audit flag, aligning with the removal of
Stack.Spec.EnableAudit.internal/resources/gateways/caddyfile.go (1)
25-28: LGTM!The simplified condition correctly enables audit configuration based solely on broker presence, aligning with the removal of the
Stack.Spec.EnableAuditfield.api/formance.com/v1beta1/benthosstream_types.go (1)
30-31: LGTM!The
ConfigMapHashfield appropriately tracks ConfigMap configuration state, supporting the new ConfigMap-driven Benthos stream architecture.internal/resources/benthosstreams/init.go (1)
42-62: LGTM!The ConfigMap creation and hash tracking logic correctly maintains configuration state for the BenthosStream.
internal/resources/gateways/gateways.go (1)
15-41: LGTM!The simplification to use fixed environment variable names (
STACK_URL,STACK_PUBLIC_URL) improves predictability and aligns with the broader removal of prefix-based helpers across the codebase.internal/tests/benthos_controller_test.go (1)
16-80: LGTM!The test simplification removes audit-specific testing in favor of focusing on core Benthos functionality, aligning with the removal of stack-level audit gating throughout the codebase.
internal/resources/gateways/deployment.go (2)
13-20: LGTM: improved readability.Multi-line function signature improves readability for this parameter-heavy function.
25-32: LGTM: simplified broker gating.The removal of
stack.Spec.EnableAuditcheck aligns with the PR's objective to simplify audit-related logic. Broker environment variables are now consistently added whenever a broker is available.internal/resources/benthos/controller.go (4)
4-7: LGTM: embedded templates and hashing support.The new imports and embedded FS setup enable dynamic template loading and config hash computation, which aligns with the refactoring to ConfigMap-driven templates.
Also applies to: 29-30
78-83: Migration code tracked for removal.This cleanup code for pre-v3.0.0 resources is appropriately marked with a TODO for removal in the next minor version.
152-164: LGTM: cleanup of legacy config map.This migration code appropriately removes the old
benthos-auditConfigMap from pre-v3.0.0 deployments.
206-242: LGTM: clean ConfigMap and deployment orchestration.The refactored approach to dynamically generate ConfigMaps, compute content-based hashes, and wire volumes is well-structured. The config-hash annotation ensures deployments roll when configuration changes.
Also applies to: 276-330
cd0ad80 to
c7708ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
111-111: Use a proper markdown heading.The section title should be a markdown heading for correct document structure and navigation.
Apply this diff to fix the heading:
-**Enable audit for a stack** +### Enable audit for a stack
🧹 Nitpick comments (1)
internal/resources/benthos/controller.go (1)
251-260: Consider returning error instead of using panic for JSON encoding.While
map[string]stringshould reliably encode to JSON, usingpanic()at line 254 is not idiomatic when the function already has error return capability. If encoding fails due to an unexpected runtime condition, it's better to return an error than crash the operator.Apply this diff to handle the error gracefully:
digest := sha256.New() for _, configMap := range configMaps { if err := json.NewEncoder(digest).Encode(configMap.Data); err != nil { - panic(err) + return errors.Wrap(err, "encoding config map data for hash") } } for _, stream := range streams { digest.Write([]byte(stream.Status.ConfigMapHash)) } configHash := base64.StdEncoding.EncodeToString(digest.Sum(nil))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (16)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stacks.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthosstreams.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockinternal/resources/benthos/builtin-templates/event_bus.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (33)
README.md(1 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/benthosstream_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/stack_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(1 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(2 hunks)internal/core/reconciler.go(1 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(7 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/benthosstreams/init.go(2 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/gateways/Caddyfile.gotpl(1 hunks)internal/resources/gateways/caddyfile.go(1 hunks)internal/resources/gateways/deployment.go(2 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/gateways/init.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/webhooks/deployment.go(0 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(3 hunks)internal/tests/gateway_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (13)
- internal/resources/searches/clean_legacy_consumers.go
- internal/tests/searches_controller_test.go
- internal/resources/ledgers/assets.go
- docs/09-Configuration reference/01-Settings.md
- internal/tests/payments_controller_test.go
- internal/resources/payments/init.go
- internal/resources/webhooks/deployment.go
- api/formance.com/v1beta1/search_types.go
- api/formance.com/v1beta1/stack_types.go
- internal/resources/searches/benthos/fs.go
- internal/resources/benthosstreams/create.go
- internal/resources/searches/init.go
- internal/tests/ledger_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/resources/auths/init.go
- api/formance.com/v1beta1/benthosstream_types.go
- internal/resources/gateways/Caddyfile.gotpl
- internal/resources/gateways/init.go
- internal/tests/gateway_controller_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.457Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
📚 Learning: 2025-11-12T15:18:19.457Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.457Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
Applied to files:
internal/resources/benthosstreams/init.goapi/formance.com/v1beta1/benthos_types.goapi/formance.com/v1beta1/zz_generated.deepcopy.gointernal/tests/benthos_controller_test.gointernal/resources/ledgers/init.gointernal/resources/benthos/controller.go
📚 Learning: 2025-11-12T15:17:06.151Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthos/controller.go:186-195
Timestamp: 2025-11-12T15:17:06.151Z
Learning: In the formancehq/operator repository, when reading embedded files (using embed.FS) that are owned and controlled by the team, using panic() for read errors is acceptable since these files must be properly formatted and present at build time.
Applied to files:
internal/resources/ledgers/init.gointernal/resources/benthos/controller.go
🧬 Code graph analysis (8)
internal/resources/benthosstreams/init.go (2)
internal/resources/databases/init.go (1)
Delete(105-137)internal/core/utils.go (1)
HashFromConfigMaps(27-35)
internal/core/reconciler.go (1)
api/formance.com/v1beta1/shared.go (1)
Object(294-300)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/resources/gateways/deployment.go (4)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/broker_types.go (4)
Broker(59-65)Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)internal/resources/gateways/gateways.go (1)
GetEnvVars(28-41)internal/core/env.go (2)
GetDevEnvVars(68-77)Env(11-16)
internal/tests/benthos_controller_test.go (2)
api/formance.com/v1beta1/stack_types.go (1)
Stack(72-78)api/formance.com/v1beta1/settings_types.go (1)
Settings(208-213)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/orchestrations/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/benthos/controller.go (4)
internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/core/env.go (1)
Env(11-16)internal/resources/settings/helpers.go (2)
IsTrue(462-464)Get(20-38)internal/core/utils.go (1)
CreateOrUpdate(123-139)
🪛 markdownlint-cli2 (0.18.1)
README.md
111-111: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (13)
internal/resources/brokerconsumers/create.go (1)
11-11: LGTM! Import standardization improves consistency.Removing the import alias and using the default package name simplifies the code and aligns with the broader cleanup effort in this PR.
internal/core/reconciler.go (1)
138-138: LGTM!The parameter rename from
buildertobis cosmetic and improves naming consistency with similar handler functions elsewhere in the file (lines 148, 165).internal/resources/orchestrations/init.go (1)
66-66: LGTM! Version gate removal aligns with v1 deprecation.The removal of the
IsGreaterOrEqual(version, "v2.0.0-rc.5")check simplifies the migration logic to run whenever the saved module version differs from the target version. This aligns with the PR's objective to remove v1 support entirely.internal/resources/webhooks/init.go (1)
70-74: LGTM! Clean removal of version-based deployment branching.The elimination of the version check simplifies the reconcile flow and aligns with the PR's goal of removing v1 support. The unconditional call to
createSingleDeploymentwhen the consumer is ready is appropriate.internal/resources/benthosstreams/init.go (1)
42-62: LGTM! Proper ConfigMap management and hash derivation.The ConfigMap creation follows the standard CreateOrUpdate pattern, and the ConfigMapHash is correctly derived using the
HashFromConfigMapshelper. The explicit return ofnilon success is clear.internal/tests/benthos_controller_test.go (1)
51-52: LGTM! Simplified broker DSN setup.The test setup now uses
settings.Newto create the broker DSN setting with a noop DSN, which is appropriate for test scenarios. This aligns with the removal of audit-specific test contexts.api/formance.com/v1beta1/benthos_types.go (1)
32-33: LGTM! Clean API surface change for dynamic configuration.The addition of
ResourcesandTemplatesmaps provides flexibility for dynamic Benthos configuration, replacing the previousBatchingfield. Themap[string]stringtype is appropriate for key-value configuration data.api/formance.com/v1beta1/zz_generated.deepcopy.go (1)
343-356: LGTM! Standard generated deepcopy logic.The deepcopy implementation for the new
ResourcesandTemplatesmaps follows the standard pattern formap[string]stringtypes. This is auto-generated code from controller-gen.internal/resources/gateways/caddyfile.go (1)
25-28: LGTM! Simplified audit enablement logic.The condition now depends solely on broker presence rather than an explicit
EnableAuditflag. This simplification is consistent with the removal of theEnableAuditfield from the API surface and makes audit capability directly tied to broker availability.README.md (1)
113-153: LGTM! Helpful audit setup documentation.The example demonstrates the multi-resource setup needed for audit functionality, including BrokerConsumer, Benthos, and BenthosStream configurations. The YAML structure aligns with the API changes, and the placeholder
<stack-name>appropriately indicates where users need to customize.internal/resources/gateways/gateways.go (1)
28-41: LGTM! Simplified to fixed environment variable names.The removal of prefix-based env var naming in favor of fixed names (
STACK_URLandSTACK_PUBLIC_URL) improves predictability and reduces complexity. The URL construction logic remains correct.docs/09-Configuration reference/02-Custom Resource Definitions.md (2)
710-711: LGTM!The updated Ledger description accurately reflects the module's purpose and aligns with the v2-focused architecture after removing v1 support.
1614-1615: LGTM!The new
resourcesandtemplatesfields are correctly documented and align with the code changes that introduce dynamic ConfigMap-based configuration for Benthos.
c7708ce to
5df88b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
111-153: Promote the audit section title to a real headingMarkdownlint still flags the bold title (MD036). Please switch it to a proper heading for structure/navigation.
-**Enable audit for a stack** +### Enable audit for a stack
🧹 Nitpick comments (2)
internal/resources/benthos/controller.go (2)
78-83: Track the cleanup TODO for next minor release.The cleanup code for the pre-v3.0.0 elasticsearch resource reference includes a TODO comment to remove this in the next minor version. Ensure this is tracked and removed as planned to avoid accumulating technical debt.
251-260: Consider error handling for config hash computation.The panic at line 254 occurs when JSON-encoding
configMap.Data(amap[string]string) fails. While this should rarely happen with well-formed string maps, encoding runtime data from the Kubernetes API differs from embedded build-time assets. Consider whether graceful error handling would be more appropriate here, especially since the function is part of a reconciliation loop where transient errors can be retried.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (17)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stacks.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthosstreams.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_stacks.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockinternal/resources/benthos/builtin-templates/event_bus.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (33)
README.md(1 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/benthosstream_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/stack_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(1 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(3 hunks)internal/core/reconciler.go(1 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(7 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/benthosstreams/init.go(2 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/gateways/Caddyfile.gotpl(1 hunks)internal/resources/gateways/caddyfile.go(1 hunks)internal/resources/gateways/deployment.go(2 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/gateways/init.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/webhooks/deployment.go(0 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(3 hunks)internal/tests/gateway_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (13)
- api/formance.com/v1beta1/search_types.go
- internal/tests/searches_controller_test.go
- internal/resources/searches/init.go
- internal/tests/payments_controller_test.go
- internal/resources/payments/init.go
- internal/resources/searches/clean_legacy_consumers.go
- internal/tests/ledger_controller_test.go
- internal/resources/ledgers/assets.go
- api/formance.com/v1beta1/stack_types.go
- internal/resources/webhooks/deployment.go
- docs/09-Configuration reference/01-Settings.md
- internal/resources/benthosstreams/create.go
- internal/resources/searches/benthos/fs.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/resources/gateways/Caddyfile.gotpl
- internal/resources/orchestrations/init.go
- internal/resources/gateways/deployment.go
- internal/resources/brokerconsumers/create.go
- internal/resources/gateways/init.go
- internal/tests/gateway_controller_test.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.481Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
📚 Learning: 2025-11-12T15:18:19.481Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.481Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
Applied to files:
api/formance.com/v1beta1/benthos_types.goapi/formance.com/v1beta1/benthosstream_types.gointernal/resources/ledgers/init.gointernal/resources/benthos/controller.goapi/formance.com/v1beta1/zz_generated.deepcopy.gointernal/tests/benthos_controller_test.gointernal/resources/benthosstreams/init.go
📚 Learning: 2025-11-12T15:17:06.182Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthos/controller.go:186-195
Timestamp: 2025-11-12T15:17:06.182Z
Learning: In the formancehq/operator repository, when reading embedded files (using embed.FS) that are owned and controlled by the team, using panic() for read errors is acceptable since these files must be properly formatted and present at build time.
Applied to files:
internal/resources/ledgers/init.gointernal/resources/benthos/controller.go
🧬 Code graph analysis (7)
api/formance.com/v1beta1/benthosstream_types.go (1)
api/formance.com/v1beta1/shared.go (1)
Status(224-233)
internal/resources/auths/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/benthos/controller.go (4)
internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/core/env.go (1)
Env(11-16)internal/resources/settings/helpers.go (2)
IsTrue(462-464)Get(20-38)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/tests/benthos_controller_test.go (4)
api/formance.com/v1beta1/benthos_types.go (1)
Benthos(50-56)api/formance.com/v1beta1/broker_types.go (1)
Broker(59-65)api/formance.com/v1beta1/stack_types.go (1)
Stack(72-78)api/formance.com/v1beta1/settings_types.go (1)
Settings(208-213)
internal/resources/benthosstreams/init.go (2)
internal/resources/databases/init.go (1)
Delete(105-137)internal/core/utils.go (1)
HashFromConfigMaps(27-35)
🪛 markdownlint-cli2 (0.18.1)
README.md
111-111: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (16)
internal/resources/auths/init.go (1)
85-93: LGTM! Migration trigger logic simplified correctly.The removal of version-based gating in favor of a simple inequality check aligns well with the PR objective to remove v1 support. The migration logic correctly handles all scenarios:
- New installations (empty saved version): migration runs
- Version match: migration skipped
- Version mismatch: migration runs and saves the new version
internal/resources/webhooks/init.go (1)
71-73: Verification successful: Dual deployment logic fully removed from webhooks package.The unconditional use of
createSingleDeploymentcorrectly simplifies the reconciliation logic by removing the version-gated branching. Verification confirms thatcreateDualDeploymentandcreateWorkerDeploymenthave been completely removed from the webhooks package. The remaining reference tocreateWorkerDeploymentin the payments package is unrelated.internal/resources/gateways/caddyfile.go (1)
25-28: LGTM! Simplified audit enablement logic.The removal of the
stack.Spec.EnableAuditcheck simplifies the logic—audit is now enabled whenever a broker is present, which aligns with the removal of the EnableAudit field from the API as part of v1 deprecation.internal/resources/gateways/gateways.go (1)
15-41: LGTM! Consolidation to fixed env var names.The removal of prefix-based env var construction in favor of fixed names (
STACK_URLandSTACK_PUBLIC_URL) simplifies the interface and aligns with the broader consolidation effort across the codebase mentioned in the summary. The logic correctly handles the conditional addition ofSTACK_PUBLIC_URLwhen ingress configuration is present.internal/core/reconciler.go (1)
138-138: LGTM! Parameter rename improves consistency.The parameter rename from
buildertobis consistent with other similar handler functions in this file (lines 148, 165) and has no behavioral impact. Verified that the removedWithReconcilerfunction has no remaining references in the codebase.internal/resources/ledgers/init.go (1)
77-124: Migration flow simplification looks solidThe streamlined migration + install handoff reads cleanly and keeps the fallback logic easy to follow. 👍
api/formance.com/v1beta1/benthosstream_types.go (1)
29-32: Status extension matches the controller contractAdding
ConfigMapHashto the status matches the new reconciliation flow and gives us an explicit change detector. Looks good.api/formance.com/v1beta1/benthos_types.go (1)
24-34: Resources/Templates maps are a nice additionThe new maps give us a clean way to ship embedded configs without piling on bespoke structs. Nicely done.
internal/tests/benthos_controller_test.go (1)
60-77: Happy-path assertions updated correctlyGreat to see the test now covering both templates/resources ConfigMaps—matches the controller’s responsibilities.
internal/resources/benthosstreams/init.go (1)
32-62: BenthosStream reconcile overhaul looks goodLegacy cleanup + direct ConfigMap management keeps streams tidy, and the ConfigMap hash wiring matches the status change. 👍
internal/resources/benthos/controller.go (3)
176-191: Embedded file reads are correct.The panic() calls for embedded file read errors are acceptable here since these files are owned and controlled by the team at build time. Any errors indicate a serious build/packaging issue that should halt execution.
Based on learnings
160-232: LGTM: Clean ConfigMap and volume construction.The loop-based approach to construct ConfigMaps, volumes, and mounts is well-structured. The direct assignment of
object.filestot.Dataat line 201 is correct since both aremap[string]string, avoiding the previous YAML marshaling issues withjson.RawMessage.
105-132: LGTM: Environment variable configuration is correct.The broker configuration environment variables are properly constructed from
broker.Status.URI, with appropriate handling of scheme-specific settings (Kafka TLS/SASL, NATS bind mode) based on query parameters.docs/09-Configuration reference/02-Custom Resource Definitions.md (3)
709-710: LGTM: Ledger description updated appropriately.The updated description accurately reflects the ledger as a stateful application managing transactions and audit trails, removing references to the deprecated deployment strategies.
1613-1614: LGTM: BenthosSpec documentation matches API changes.The documentation correctly reflects the new
resourcesandtemplatesfields (bothmap[string]string), which replaced the previous batching configuration.
1731-1731: LGTM: BenthosStreamStatus documentation complete.The
configMapHashfield documentation is consistent with the implementation that tracks configuration changes via SHA-256 hash.
5df88b7 to
2174026
Compare
2174026 to
73f25d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
README.md (1)
111-111: Use a markdown heading instead of bold text.The section title should be a proper markdown heading for better document structure and navigation.
Apply this diff:
-**Enable audit for a stack** +### Enable audit for a stackinternal/resources/gateways/deployment.go (1)
64-77: Provide a fallback when broker mode is unset.If
broker.Status.Modeis empty or an unexpected value (common before status reconciliation or when introducing a new mode), the switch leavestopicPrefixempty and we emitTOPIC_NAME=gateway. That leaks events across stacks sharing the broker because the topic loses its per-stack prefix. Add a default branch that fails fast or derives a deterministic prefix (e.g.,stack.Name + "-") so we never send traffic to an unprefixed topic. Same issue as previously flagged.Apply this diff to set a safe fallback:
switch broker.Status.Mode { case v1beta1.ModeOneStreamByService: topicPrefix = broker.Spec.Stack + "-" case v1beta1.ModeOneStreamByStack: topicPrefix = broker.Spec.Stack + "." + default: + topicPrefix = stack.Name + "-" }internal/resources/benthos/controller.go (1)
251-264: Replace panic with error return in hash computation.The panic on line 254 when JSON encoding fails will crash the entire operator. While encoding a
map[string]stringshould rarely fail, defensive error handling is better practice. The function should return an error that the caller can handle appropriately.Apply this diff to return errors instead of panicking:
- digest := sha256.New() - for _, configMap := range configMaps { - if err := json.NewEncoder(digest).Encode(configMap.Data); err != nil { - panic(err) - } - } - for _, stream := range streams { - digest.Write([]byte(stream.Status.ConfigMapHash)) - } - configHash := base64.StdEncoding.EncodeToString(digest.Sum(nil)) + digest := sha256.New() + for _, configMap := range configMaps { + if err := json.NewEncoder(digest).Encode(configMap.Data); err != nil { + return errors.Wrap(err, "encoding configmap data for hash") + } + } + for _, stream := range streams { + digest.Write([]byte(stream.Status.ConfigMapHash)) + } + configHash := base64.StdEncoding.EncodeToString(digest.Sum(nil))
🧹 Nitpick comments (2)
docs/09-Configuration reference/02-Custom Resource Definitions.md (2)
1613-1614: Add descriptions for new fields.The
resourcesandtemplatesfields are documented but lack descriptions explaining their purpose and usage. Consider adding brief descriptions to help users understand what these fields are for and how they should be used.Example:
| `resources` _object (keys:string, values:string)_ | Resources define Benthos resource definitions (inputs, processors, outputs, etc.) as YAML strings. These are made available to all streams. | | | | `templates` _object (keys:string, values:string)_ | Templates define reusable Benthos configuration templates as YAML strings. Builtin templates are automatically included. | | |
1731-1731: Add description for configMapHash field.The
configMapHashfield lacks a description. Consider adding a brief explanation of its purpose.Example:
| `configMapHash` _string_ | ConfigMapHash stores a hash of the stream's ConfigMap data, used to trigger pod restarts when configuration changes. | | |
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (17)
config/crd/bases/formance.com_benthos.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_benthosstreams.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_searches.yamlis excluded by!**/*.yamlconfig/crd/bases/formance.com_stacks.yamlis excluded by!**/*.yamlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthos.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_benthosstreams.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_searches.formance.com.yamlis excluded by!**/*.yamlhelm/crds/templates/crds/apiextensions.k8s.io_v1_customresourcedefinition_stacks.formance.com.yamlis excluded by!**/*.yamlhelm/operator/Chart.lockis excluded by!**/*.lock,!**/*.lockinternal/resources/benthos/builtin-templates/event_bus.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_accounts.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_all.yamlis excluded by!**/*.yamlinternal/resources/ledgers/assets/reindex/v2.0.0/ledger_reindex_transactions.yamlis excluded by!**/*.yamlinternal/resources/searches/benthos/audit/gateway_audit.yamlis excluded by!**/*.yaml
📒 Files selected for processing (33)
README.md(1 hunks)api/formance.com/v1beta1/benthos_types.go(1 hunks)api/formance.com/v1beta1/benthosstream_types.go(1 hunks)api/formance.com/v1beta1/search_types.go(0 hunks)api/formance.com/v1beta1/stack_types.go(0 hunks)api/formance.com/v1beta1/zz_generated.deepcopy.go(1 hunks)docs/09-Configuration reference/01-Settings.md(0 hunks)docs/09-Configuration reference/02-Custom Resource Definitions.md(3 hunks)internal/core/reconciler.go(1 hunks)internal/resources/auths/init.go(1 hunks)internal/resources/benthos/controller.go(7 hunks)internal/resources/benthosstreams/create.go(0 hunks)internal/resources/benthosstreams/init.go(2 hunks)internal/resources/brokerconsumers/create.go(1 hunks)internal/resources/gateways/Caddyfile.gotpl(2 hunks)internal/resources/gateways/caddyfile.go(1 hunks)internal/resources/gateways/deployment.go(2 hunks)internal/resources/gateways/gateways.go(1 hunks)internal/resources/gateways/init.go(1 hunks)internal/resources/ledgers/assets.go(0 hunks)internal/resources/ledgers/init.go(4 hunks)internal/resources/orchestrations/init.go(1 hunks)internal/resources/payments/init.go(0 hunks)internal/resources/searches/benthos/fs.go(0 hunks)internal/resources/searches/clean_legacy_consumers.go(0 hunks)internal/resources/searches/init.go(0 hunks)internal/resources/webhooks/deployment.go(0 hunks)internal/resources/webhooks/init.go(1 hunks)internal/tests/benthos_controller_test.go(3 hunks)internal/tests/gateway_controller_test.go(1 hunks)internal/tests/ledger_controller_test.go(0 hunks)internal/tests/payments_controller_test.go(0 hunks)internal/tests/searches_controller_test.go(0 hunks)
💤 Files with no reviewable changes (13)
- internal/tests/ledger_controller_test.go
- internal/resources/payments/init.go
- internal/resources/searches/init.go
- internal/resources/searches/clean_legacy_consumers.go
- internal/tests/payments_controller_test.go
- api/formance.com/v1beta1/search_types.go
- docs/09-Configuration reference/01-Settings.md
- internal/tests/searches_controller_test.go
- api/formance.com/v1beta1/stack_types.go
- internal/resources/benthosstreams/create.go
- internal/resources/searches/benthos/fs.go
- internal/resources/webhooks/deployment.go
- internal/resources/ledgers/assets.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/resources/auths/init.go
- internal/resources/gateways/init.go
- internal/resources/brokerconsumers/create.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.481Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
📚 Learning: 2025-11-12T15:18:19.481Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthosstreams/init.go:34-40
Timestamp: 2025-11-12T15:18:19.481Z
Learning: In the formancehq/operator repository, the operator stopped creating BenthosStream objects as part of removing v1 support. Any BenthosStream resources with owner references from formance.com/v1beta1 components are legacy and should be cleaned up during the migration period.
Applied to files:
internal/resources/benthos/controller.gointernal/resources/ledgers/init.goapi/formance.com/v1beta1/benthos_types.gointernal/resources/benthosstreams/init.goapi/formance.com/v1beta1/benthosstream_types.go
📚 Learning: 2025-11-12T15:17:06.182Z
Learnt from: gfyrag
Repo: formancehq/operator PR: 294
File: internal/resources/benthos/controller.go:186-195
Timestamp: 2025-11-12T15:17:06.182Z
Learning: In the formancehq/operator repository, when reading embedded files (using embed.FS) that are owned and controlled by the team, using panic() for read errors is acceptable since these files must be properly formatted and present at build time.
Applied to files:
internal/resources/benthos/controller.gointernal/resources/ledgers/init.go
🧬 Code graph analysis (8)
internal/tests/benthos_controller_test.go (1)
api/formance.com/v1beta1/settings_types.go (1)
Settings(208-213)
internal/resources/benthos/controller.go (4)
internal/resources/resourcereferences/create.go (1)
Delete(46-55)internal/core/env.go (1)
Env(11-16)internal/resources/settings/helpers.go (2)
IsTrue(462-464)Get(20-38)internal/core/utils.go (1)
CreateOrUpdate(123-139)
internal/resources/ledgers/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
internal/resources/benthosstreams/init.go (2)
internal/resources/databases/init.go (1)
Delete(105-137)internal/core/utils.go (1)
HashFromConfigMaps(27-35)
internal/resources/gateways/gateways.go (1)
api/formance.com/v1beta1/gateway_types.go (1)
Gateway(76-82)
internal/resources/gateways/deployment.go (7)
internal/core/context.go (1)
Context(10-16)api/formance.com/v1beta1/broker_types.go (4)
Broker(59-65)Mode(28-28)ModeOneStreamByService(31-31)ModeOneStreamByStack(32-32)internal/resources/gateways/gateways.go (1)
GetEnvVars(28-41)internal/core/env.go (2)
GetDevEnvVars(68-77)Env(11-16)internal/resources/brokers/utils.go (1)
GetBrokerEnvVars(16-68)internal/core/stacks.go (1)
HasDependency(69-80)internal/resources/gateways/url.go (1)
URL(9-15)
api/formance.com/v1beta1/benthosstream_types.go (1)
api/formance.com/v1beta1/shared.go (1)
Status(224-233)
internal/resources/orchestrations/init.go (1)
internal/resources/databases/create.go (1)
GetSavedModuleVersion(69-71)
🪛 markdownlint-cli2 (0.18.1)
README.md
111-111: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (20)
internal/core/reconciler.go (1)
138-138: LGTM! Parameter rename improves consistency.The parameter rename from
buildertobaligns with the naming convention used in similar functions at lines 148 and 165, improving code consistency.internal/resources/orchestrations/init.go (1)
66-75: Remove version gate introduces downgrade vulnerability—clarify intent and ensure migration safety.This change removes the
IsGreaterOrEqual(version, "v2.0.0-rc.5")guard, so migrations now trigger on any version mismatch. Analysis reveals:
No downgrade protection: Codebase contains no explicit downgrade handling. With simple
!=comparison, a rollback (saved version > current version) will still attempt migration, which may fail or corrupt data since migrations typically assume forward progression only.Inconsistent across modules: Other modules retain the version gate:
internal/resources/webhooks/init.go:59— still usesIsGreaterOrEqual()internal/resources/reconciliations/init.go:54— still usesIsGreaterOrEqual()- But orchestrations, ledgers, payments, auths do not
First deployment: An empty saved version (first deployment) unconditionally triggers migration—verify this is intended.
Clarify:
- Is this intentional as part of v1 removal, or an oversight?
- Are migrations guaranteed safe for re-runs and downgrades, or does the container-level "migrate" command handle these?
- Why do some modules retain version gates while others don't?
internal/resources/ledgers/init.go (2)
107-117: Clarify the version-specific error handling logic.Based on past review comments, there's uncertainty about whether this error recovery (starting the ledger despite migration failures) should apply to all v2.2+ versions or only specific versions.
The current logic allows the ledger to start for v2.2+ even when migrations fail with application errors. Please confirm this is intentional for all v2.2+ versions.
84-104: The deletion pattern is intentional and safe.The PreCreate hook deletes all deployments owned by the ledger before migrations because downtime during schema migrations is expected. The flow is: (1) uninstall old deployment types, (2) delete remaining ledger-owned deployments via PreCreate, (3) run migration job, (4) recreate deployments via subsequent
installLedger()call. Using owner references ensures deletion is scoped only to this ledger resource. This pattern matches the payments resource implementation and follows established practices in the codebase.api/formance.com/v1beta1/zz_generated.deepcopy.go (1)
343-356: LGTM!The auto-generated deepcopy logic for the new
ResourcesandTemplatesmaps inBenthosSpecis correct.internal/tests/gateway_controller_test.go (1)
196-196: LGTM!The test context has been appropriately renamed to reflect the new approach where audit is enabled via broker consumer presence rather than an explicit
EnableAuditflag.internal/resources/webhooks/init.go (1)
71-73: LGTM!The removal of v2 gating logic simplifies the deployment flow. Webhooks now always use the single deployment approach.
internal/resources/gateways/caddyfile.go (1)
25-28: LGTM!The audit configuration is now enabled based solely on broker presence, removing the dependency on the deprecated
EnableAuditflag. This aligns with the API changes in this PR.api/formance.com/v1beta1/benthosstream_types.go (1)
31-31: LGTM!The new
ConfigMapHashfield enables tracking configuration changes for BenthosStream resources, supporting hash-based reconciliation.README.md (1)
113-153: LGTM!The new audit documentation clearly demonstrates the multi-resource approach using BrokerConsumer, Benthos, and BenthosStream, which aligns with the architectural changes in this PR.
internal/resources/gateways/Caddyfile.gotpl (2)
14-20: LGTM!The new audit configuration directives provide comprehensive control over audit behavior, including topic routing, authentication, and organizational context.
62-62: Verify the audit directive order change.The audit order changed from "after encode" to "before handle". This means audit now runs earlier in the Caddy handler chain, before the main request handler.
Please confirm this reordering is intentional and that audit should capture requests before they're handled (rather than after encoding responses).
docs/09-Configuration reference/02-Custom Resource Definitions.md (1)
709-710: Documentation accurately reflects v1 removal.The updated description correctly removes references to deployment strategies and provides a clearer, simpler explanation of the Ledger module's purpose.
internal/resources/benthos/controller.go (7)
4-7: LGTM! Clean integration of embedded templates.The addition of crypto/sha256, embed, base64, and json imports along with the embedded builtin-templates filesystem is appropriate for the new configuration hash computation and template inclusion features.
Also applies to: 29-30
78-83: Migration cleanup is appropriate.The removal of the old elasticsearch resource reference is consistent with the v1 removal objectives. The TODO comment correctly notes this can be removed in the next minor version after users have migrated.
85-95: LGTM! Debug statements successfully removed.The broker readiness check is correctly implemented, and the debug println statements from previous review have been removed.
105-132: LGTM! Streamlined environment variable configuration.The environment variable setup correctly derives broker configuration from
broker.Status.URI, properly handling Kafka TLS/SASL settings and NATS binding mode. The approach is cleaner than prefix-based helpers.
142-154: Migration cleanup is appropriate.The deletion of the old benthos-audit ConfigMap handles the migration from pre-v3.0.0 correctly, with proper error handling and context wrapping.
156-232: LGTM! Clean ConfigMap generation with builtin templates.The ConfigMap generation correctly handles resources and templates, including the automatic inclusion of builtin templates from the embedded filesystem. The direct assignment of
object.filestot.Datais correct (the previous yaml.Marshal issue was addressed). Based on learnings, the panic on embedded file read errors is acceptable since these files are controlled by the team.Based on learnings
266-319: LGTM! Deployment properly integrates config hash and volumes.The deployment correctly uses the computed config hash in pod annotations to trigger restarts on configuration changes, and properly mounts all resources, templates, and stream ConfigMaps. The volume projection approach for streams is clean and maintainable.
4 participants
BREAKING CHANGE: This removes support for Ledger v1 deployments. All ledgers will now use v2 behavior by default.